[#154] Add UI-based unit tests for demo application #197

Tayebsed93 · 2024-10-16T11:46:10Z

Note: Please transform - [ ] into - (NA) in the description when things are not applicable

Related issues

#154

Description

Add unit tests for screenshot comparisons

Motivation & Context

Types of change

New feature (non-breaking change which adds functionality)

Previews

Checklist

Contribution

I have read the contributing guidelines

Accessibility

(NA) My change follows accessibility good practices

Design

(NA) My change respects the design guidelines of Orange Unified Design System

Development

My change follows the developer guide
I checked my changes do not add new SwiftLint warnings
I have added unit tests to cover my changes (optional)

Documentation

My change introduces changes to the documentation and/or I have updated the documentation accordingly

Checklist (for Core Team only)

(NA) The evolution have been tested and the project builds for iPhones and iPads
Code review has been done by reviewers according to CODEOWNERS file
(NA) Design review has been done
(NA) Accessibiltiy review has been done
(NA) Q/A team has tested the evolution
Documentation has been updated if relevant
Internal files have been updated if relevant (like CONTRIBUTING, DEVELOP, THIRD_PARTY, CONTRIBUTORS, NOTICE)
CHANGELOG has been updated respecting keep a changelog rules and reference the issues

Tayebsed93 · 2024-10-17T15:46:51Z

@pylapp Merci pour ton retour. Désormais, j'arrive à voir les warnings et plus de 250 d'entre eux sont corrigés. Les fonctions de test ont aussi été modifiées. Le rebase est fait

pylapp

La PR est loin d'être acceptable en l'état.

Il ne faut pas que tu corriges bêtement les warnings sans faire attention à ce que tu fais, surtout quand tu apportes de l'incohérence entre les fichiers du projet.

Regarde des classes de tests à côté ; tu pourras grandement t'en inspirer.

Dans les points à corriger :

Il faut des sauts de lignes entre les fonctions, c'est la base ;
le warning que tu as voulu corriger ne te dit pas de supprimer la ligne, mais les espaces inutiles en début de ligne ;
Pour les classes de tests on n'a pas besoin de deinit, pour le coup tu peux désactiver proprement ce warning comme indiqué en commentaire ;
Tu as réussi à mettre des fonctions de tests en dehors de leur classe de test ;
On ajoute les nouvelles lignes du CHANGELOG par le haut ; ta ligne est totalement perdue dans une section ;
Aucune documentation, tant pour les classes de tests que dans le DEVELOP par exemple ;
Les MARKS sont utilisés n'importe comment ;
Pourquoi on ne teste parfois que pour le color scheme light et pas le dark aussi ?
Tu rajoutes du code qui ne semble n'avoir aucun rapport avec ce que tu dois faire dans ta PR.

Un MARK est une balise qui sert d'ancrage dans la minimap de Xcode, je te laisse chercher. Si tu ajoutes après les : un tiret, une ligne horizontale apparait pour séparer visuellement les choses. L'intérêt des MARK est d'apporter une aide visuelle dans le code, avec une structure claire. Deux MARK qui se suivent ne servent à rien.

Par exemple, si je prends :

    // MARK: - Tests
    // MARK: - Orange Theme Light Mode Width Tests
    // MARK: - Width Tokens Tests

Le seul MARK utile est ici :

    // MARK: - Orange Theme Light Mode Width Tests

D'ailleurs, je ne comprends pas comment pour un même code modulo la valeur du paramètre "named" tu peux définir des cas de tests différents.
Il faudrait que tu précises à la fin de la rubrique "UI tests in demo app" de DEVELOP comment ça fonctionne. Si je ne comprends pas, d'autres ne comprendront pas. Si on doit te demander comment ça fonctionne, c'est qu'il y a un problème a minima de documentation quelque part. Si tu n'es pas à l'aise en anglais pour expliquer clairement comment ça fonctionne, rédige le paragraphe en français en commentaire de cette PR et je traduirais.

Au besoin, demande de l'aide à @ludovic35 qui avait travaillé sur le sujet.

pylapp · 2024-10-17T19:59:10Z

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [Library] Add semantic tokens `spacePaddingInlineTallest`, `spacePaddingBlockTallest`, `spaceColumnGapTaller`, `spaceColumnGapWithArrowShortest`, `spaceRowGapShortest` (Figjam final synchronization of October 16th)
 - [Library] Add `elevationX200` raw token (Figjam final synchronization of October 16th)
 - [Library] Add semantic color tokens ([#124](https://github.com/Orange-OpenSource/ouds-ios/issues/124))
+- [DemoApp] Add more UI tests on demo app ([#154](https://github.com/Orange-OpenSource/ouds-ios/issues/154))


Il faut mettre la nouvelle ligne du CHANGELOG en haut de la section appropriée

pylapp · 2024-10-17T20:02:15Z

Showcase/Showcase/Pages/ThemeSelection/ThemeSelection.swift

@@ -114,6 +114,7 @@ struct ThemeSelectionButton: View {
 .pickerStyle(.automatic)
 } label: {
 Image(systemName: "paintpalette")
+ .accessibilityLabel("app_common_theme_image_a11y")


Que fait cette ligne de code ici ? Je ne vois pas le rapport avec les tests UI que tu implémentes. Si ça n'a pas de rapport, il faut retirer. Si tu as décelé un bug, il faut créer une issue.

pylapp · 2024-10-17T20:03:37Z

Showcase/Showcase/Pages/Utils/ShowcaseElementsPage.swift

@@ -38,6 +38,7 @@ struct ShowcaseElementsPage: View {
 Card(
 title: Text(LocalizedStringKey(element.name)),
 icon: Image(element.imageName))
+ .accessibilityLabel(element.id)


D'où vient cette ligne de code ? Si ça n'a pas de rapport avec le but initial de ta PR il faut retirer. Si tu as décelé un bug, il faut faire une issue

pylapp · 2024-10-17T20:04:08Z

Showcase/Showcase/Resources/en.lproj/Localizable.strings

@@ -11,6 +11,9 @@
 // Software description: A SwiftUI components library with code examples for Orange Unified Design System
 //

+// MARK: - Common


D'où vient cette ligne de code ? Si ça n'a pas de rapport avec le but initial de ta PR il faut retirer. Si tu as décelé un bug, il faut faire une issue

Par ailleurs, il faut un saut de ligne après le MARK, comme c'est le cas ailleurs dans le fichier.

pylapp · 2024-10-17T20:04:45Z

Showcase/ShowcaseTests/OUDSComponentsUITests/OUDSComponentsUITests.swift

+// Software description: A SwiftUI components library with code examples for Orange Unified Design System 
+//
+
+import SwiftUI


Pourquoi faire deux blocs d'imports ? Inspire toi des autres classes de tests. Il faut que tu mettes tout d'un bloc, trié alphabétiquement.

pylapp · 2024-10-17T20:10:26Z

Showcase/ShowcaseTests/OUDSTokensBorderUITests/OUDSTokensBorderUITests.swift

+ let vc = BorderTokenPage().environment(\.theme, InverseTheme())
+ assertSnapshot(of: vc, as: .image, named: "InverseTheme/testBorderToken_OrangeTheme_SectionRadius_BorderRadiusNone_Light")
+}
+func testBorderToken_InverseTheme_SectionRadius_BorderRadiusDefault_Light() {


Reste cohérent avec les autres fichiers du projet ; met des sauts de ligne entre les fonctions ! Configure au besoin ton IDE pour pas qu'il ajoute des espaces inutilement.

pylapp · 2024-10-17T20:11:04Z

Showcase/ShowcaseTests/OUDSTokensDimensionUITests/OUDSTokensDimensionUITests.swift

+import OUDSTokensSemantic
+import SnapshotTesting
+
+class OUDSTokensDimensionUITests: XCTestCase {


Il est superflu d'avoir un deinit dans les classes de tests ; regarde les autres fichiers de tests. Il y a une règle pour désactiver le warning associé : désactive-le au début de fichier, active-le à la fin, comme c'est le cas pour les autres fichiers.

Une classe doit être final, ajoute le mot clé.

Les "MARK: - Tests" n'apportent aucune valeur ici ; tu as déjà des MARK qui donnent des points d'ancrage dans la minimap, avec un trait horizontal dans l'affichage du code source.

De puis, il faut des sauts de lignes après les MARK, et entre les fonctions.
Les warnings que tu as certainement voulu corriger ne te demandaient pas de supprimer les lignes, mais de supprimes des espaces en trop en début de ligne.

Inspire toi des autres classe de test ; tu n'as même pas de documentation qui décrit le but de ce fichier.

Tu doubles à chaque fois tes MARK, ça ne sert à rien. Fais en un utile plutôt que deux qui se parasitent.

pylapp · 2024-10-17T20:11:41Z

Showcase/ShowcaseTests/OUDSTokensElevationUITests/OUDSTokensElevationUITests.swift

+import OUDSTokensSemantic
+import SnapshotTesting
+
+class OUDSTokensElevationUITests: XCTestCase {


Il est superflu d'avoir un deinit dans les classes de tests ; regarde les autres fichiers de tests. Il y a une règle pour désactiver le warning associé : désactive-le au début de fichier, active-le à la fin, comme c'est le cas pour les autres fichiers.

Une classe doit être final, ajoute le mot clé.

Les "MARK: - Tests" n'apportent aucune valeur ici ; tu as déjà des MARK qui donnent des points d'ancrage dans la minimap, avec un trait horizontal dans l'affichage du code source.

De puis, il faut des sauts de lignes après les MARK, et entre les fonctions.
Les warnings que tu as certainement voulu corriger ne te demandaient pas de supprimer les lignes, mais de supprimes des espaces en trop en début de ligne.

Inspire toi des autres classe de test ; tu n'as même pas de documentation qui décrit le but de ce fichier.

Tu doubles à chaque fois tes MARK, ça ne sert à rien. Fais en un utile plutôt que deux qui se parasitent.

pylapp · 2024-10-17T20:12:00Z

Showcase/ShowcaseTests/OUDSTokensOpacityUITests/OUDSTokensOpacityUITests.swift

+import OUDSTokensSemantic
+import SnapshotTesting
+
+class OUDSTokensOpacityUITests: XCTestCase {


Il est superflu d'avoir un deinit dans les classes de tests ; regarde les autres fichiers de tests. Il y a une règle pour désactiver le warning associé : désactive-le au début de fichier, active-le à la fin, comme c'est le cas pour les autres fichiers.

Une classe doit être final, ajoute le mot clé.

Les "MARK: - Tests" n'apportent aucune valeur ici ; tu as déjà des MARK qui donnent des points d'ancrage dans la minimap, avec un trait horizontal dans l'affichage du code source.

De puis, il faut des sauts de lignes après les MARK, et entre les fonctions.
Les warnings que tu as certainement voulu corriger ne te demandaient pas de supprimer les lignes, mais de supprimes des espaces en trop en début de ligne.

Inspire toi des autres classe de test ; tu n'as même pas de documentation qui décrit le but de ce fichier.

Tu doubles à chaque fois tes MARK, ça ne sert à rien. Fais en un utile plutôt que deux qui se parasitent.

pylapp · 2024-10-17T20:13:40Z

Showcase/ShowcaseTests/OUDSTokensTypographyUITests/OUDSTokensTypographyUITests.swift

+import OUDSTokensSemantic
+import SnapshotTesting
+
+class OUDSTokensTypographyUITests: XCTestCase {


Il est superflu d'avoir un deinit dans les classes de tests ; regarde les autres fichiers de tests. Il y a une règle pour désactiver le warning associé : désactive-le au début de fichier, active-le à la fin, comme c'est le cas pour les autres fichiers.

Une classe doit être final, ajoute le mot clé.

Les "MARK: - Tests" n'apportent aucune valeur ici ; tu as déjà des MARK qui donnent des points d'ancrage dans la minimap, avec un trait horizontal dans l'affichage du code source.

De puis, il faut des sauts de lignes après les MARK, et entre les fonctions.
Les warnings que tu as certainement voulu corriger ne te demandaient pas de supprimer les lignes, mais de supprimes des espaces en trop en début de ligne.

Inspire toi des autres classe de test ; tu n'as même pas de documentation qui décrit le but de ce fichier.

Tu doubles à chaque fois tes MARK, ça ne sert à rien. Fais en un utile plutôt que deux qui se parasitent.

Tayebsed93 requested review from pylapp and ludovic35 as code owners October 16, 2024 11:46

Tayebsed93 linked an issue Oct 16, 2024 that may be closed by this pull request

[Demo App] Add more UI tests on demo app #154

Open

15 tasks

Tayebsed93 force-pushed the 154-demo-app-add-more-ui-tests-on-demo-app branch from 949f27a to da6e278 Compare October 17, 2024 08:11

pylapp changed the title ~~154 demo app add more UI tests on demo app~~ [#154] Add UI-based uni tests for demo application Oct 17, 2024

pylapp assigned Tayebsed93 Oct 17, 2024

pylapp force-pushed the 154-demo-app-add-more-ui-tests-on-demo-app branch from da6e278 to ab3ce0a Compare October 17, 2024 13:03

pylapp marked this pull request as draft October 17, 2024 13:28

This comment was marked as outdated.

Sign in to view

pylapp changed the title ~~[#154] Add UI-based uni tests for demo application~~ [#154] Add UI-based unit tests for demo application Oct 17, 2024

Tayebsed93 force-pushed the 154-demo-app-add-more-ui-tests-on-demo-app branch 2 times, most recently from ac9c17e to bd55ec1 Compare October 17, 2024 15:43

pylapp requested changes Oct 17, 2024

View reviewed changes

Tayebsed93 force-pushed the 154-demo-app-add-more-ui-tests-on-demo-app branch from eb286c0 to 5991271 Compare October 18, 2024 14:42

Tayebsed93 added 15 commits October 18, 2024 17:08

feat : add ouds tokens border ui test

bb0c1b6

feat : add ouds tokens opacity ui tests

9f012ad

feat : add ouds token elevations ui tests

090a4cf

feat : add ouds tokens dimension ui tests

ea140d6

feat : add ouds token typography ui tests

4ea27bd

feat : add components page ui test

3cd101a

doc : update change log

8747e4f

fix : update component ui test

ed01f16

fix : podfile version

af93884

fix : theme support environment

51118c8

feat : add ouds token elevations ui tests

a892635

fix : podfile version

6c43703

review : swift lint corrections applied

c225b62

review : adjust test class names for dimension

dace9f0

review : adjust test class names for elevation

9ac7492

Tayebsed93 added 7 commits October 18, 2024 17:09

review : adjust test class names for opacity

92f3f24

review : adjust test class names for typography

102a30d

fix : delete image snapshots folder

8013618

review : fix consistency with SwiftLint rules

9f5d0d9

review : removed unrelated code

036afa1

review : create documentation ui test snapshot

145546a

review : update DEVELOP.md

aa15a4f

Tayebsed93 force-pushed the 154-demo-app-add-more-ui-tests-on-demo-app branch from 95e1b59 to aa15a4f Compare October 18, 2024 15:09

Tayebsed93 added 2 commits October 18, 2024 17:14

review : fix CHANGELOG order by moving new entry

4633104

review : add final keyword to class definitions

705e8bf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#154] Add UI-based unit tests for demo application #197

[#154] Add UI-based unit tests for demo application #197

Tayebsed93 commented Oct 16, 2024 •

edited by pylapp

Loading

This comment was marked as outdated.

Tayebsed93 commented Oct 17, 2024

pylapp left a comment •

edited

Loading

pylapp Oct 17, 2024

pylapp Oct 17, 2024

pylapp Oct 17, 2024

pylapp Oct 17, 2024

pylapp Oct 17, 2024

pylapp Oct 17, 2024

pylapp Oct 17, 2024

pylapp Oct 17, 2024

pylapp Oct 17, 2024

pylapp Oct 17, 2024

[#154] Add UI-based unit tests for demo application #197

Are you sure you want to change the base?

[#154] Add UI-based unit tests for demo application #197

Conversation

Tayebsed93 commented Oct 16, 2024 • edited by pylapp Loading

Related issues

Description

Motivation & Context

Types of change

Previews

Checklist

Contribution

Accessibility

Design

Development

Documentation

Checklist (for Core Team only)

This comment was marked as outdated.

Tayebsed93 commented Oct 17, 2024

pylapp left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tayebsed93 commented Oct 16, 2024 •

edited by pylapp

Loading

pylapp left a comment •

edited

Loading